feat(mail): couple the delivery lease to the sender's enforced timeout (HT-22) - #22
Merged
Merged
Conversation
…t (HT-22) PR #21 (HT-16) documented the invariant that the delivery lease must strictly exceed the worst-case EmailSender.send() duration, or a re-claimed retry can race a still-in-flight send into a concurrent double-send — but nothing enforced it mechanically (adversarial-review finding B's follow-up note). Now it is: EmailSender requires a declared, self-enforced per-send bound (maxSendMs — the Gmail adapter sets it from the same timeoutMs that feeds AbortSignal.timeout), and both retry paths assert maxSendMs < leaseMs via assertLeaseExceedsSenderBound BEFORE claiming a row, so a violating lease/timeout combination throws up front instead of silently re-opening the hole. specs/mail/sending.md §3a/§4 updated from convention to mechanism. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
📝 WalkthroughWalkthroughThe change adds an enforced ChangesLease-bound email delivery
Estimated code review effort: 3 (Moderate) | ~25 minutes Sequence Diagram(s)sequenceDiagram
participant DeliveryWorker
participant LeaseGuard
participant DeliveryStore
participant EmailSender
DeliveryWorker->>LeaseGuard: validate leaseMs > sender.maxSendMs
LeaseGuard-->>DeliveryWorker: allow or throw
DeliveryWorker->>DeliveryStore: listDeliverableThreads
DeliveryWorker->>DeliveryStore: claimThreadForDelivery
DeliveryWorker->>EmailSender: send email
Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/providers/adapters/gmail/sender.ts (1)
121-150: 🩺 Stability & Availability | 🟠 Major | 🏗️ Heavy lift
maxSendMsstill excludes token acquisition.getAccessToken()runs beforeAbortSignal.timeout(timeoutMs), so a slow refresh can makesend()exceed the declared bound and let a retry race a still-in-flight send. Fold token acquisition into the same timeout budget, ormaxSendMsno longer matches the worst-casesend()time.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/providers/adapters/gmail/sender.ts` around lines 121 - 150, Update createGmailEmailSender’s send method so getAccessToken() is governed by the same timeout budget as the Gmail request, ensuring the complete send operation cannot exceed maxSendMs. Create and reuse a single timeout signal before token acquisition, pass it to fetchImpl, and preserve the existing timeout behavior for the HTTP exchange.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@src/providers/adapters/gmail/sender.ts`:
- Around line 121-150: Update createGmailEmailSender’s send method so
getAccessToken() is governed by the same timeout budget as the Gmail request,
ensuring the complete send operation cannot exceed maxSendMs. Create and reuse a
single timeout signal before token acquisition, pass it to fetchImpl, and
preserve the existing timeout behavior for the HTTP exchange.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 2dc8c2e6-59fb-4850-8a78-4159518fb5d0
📒 Files selected for processing (9)
specs/mail/sending.mdsrc/api/index.test.tssrc/mail/delivery-worker.test.tssrc/mail/delivery-worker.tssrc/mail/send.test.tssrc/mail/send.tssrc/providers/adapters/gmail/sender.test.tssrc/providers/adapters/gmail/sender.tssrc/providers/email-sender.ts
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Jira: HT-22 — follow-up to #21's adversarial-review finding B.
Problem
HT-16 documented the invariant that the delivery lease (
DEFAULT_LEASE_MS = 120_000,src/mail/send.ts) must strictly exceed the worst-caseEmailSender.send()duration — otherwise a re-claimed retry can race a still-in-flight send into a concurrent double-send (specs/mail/sending.md §3a, §4). The Gmail adapter's 30 sAbortSignal.timeoutsits safely under it today, but nothing tied the two constants together: a future adapter with no/looser timeout, or a raised GmailtimeoutMs, would violate the invariant silently.Mechanical coupling
EmailSenderrequiresreadonly maxSendMs: number— the bound the implementation itself enforces on onesend()call (a real mechanical timeout, not an estimate). Required, so TypeScript rejects any future adapter that doesn't declare a bound.maxSendMs: timeoutMs— the same variable that feedsAbortSignal.timeout, so the declared bound and the enforced one cannot drift apart inside the adapter. The existing abort test proves the timeout is real; a new test pins the declaration to it (default and custom).assertLeaseExceedsSenderBound(sender, leaseMs)runs before everyclaimThreadForDelivery— insendReply's keyed retry path and at the top ofrunDeliveryWorker. Any violating combination (maxSendMs >= leaseMs; equality is a violation, the invariant says strictly) throws up front, before anything is listed, claimed, or sent. A violation is a wiring bug, so it throws rather than returning a result — matchingsendReply's "only throw on genuinely unexpected faults" contract.sending.md§3a/§4 updated from "must by convention" to "enforced mechanically".Rejected alternative: passing a lease-derived
AbortSignalintosend()— a larger contract change that guarantees nothing (an adapter can ignore the signal, and an aborted fetch may still be delivered server-side). The declared-bound + assert makes the invariant checkable exactly where lease and sender meet.Tests
maxSendMsequals the enforced timeout, default (30 000) and custom.sendReplywith a violating sender throws before claim/send (both spies uncalled); equality pinned as a violation.runDeliveryWorkerwith a violatingleaseMsthrows beforelistDeliverableThreadsis even called.Verification
On this branch (post-#21
main, including ccc9be2's claim re-check):tscexit 0,biome checkclean, full vitest suite 331/331.Noted, out of scope (in HT-22)
staleAfterMs(5 min default) has an analogous relationship to no-key sends, which never take a lease. With defaults it's transitively safe (maxSendMs < leaseMs < staleAfterMs); a separate ticket could assertmaxSendMs < staleAfterMsif that window is ever tuned aggressively low.🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Documentation